v2.6.1 — code review fixes#136
Merged
cayossarian merged 7 commits intomainfrom Apr 16, 2026
Merged
Conversation
SpanMqttClient: - snapshot_interval and set_snapshot_interval() now raise ValueError below 1.0s, removing the <=0 immediate-dispatch branch that could spawn unbounded per-message dispatch tasks on a busy broker. - Connection-callback exceptions logged at WARNING (matches _dispatch_snapshot); the old exception() level implied a fatal error that the bridge handles cleanly. - _wait_for_circuit_names uses time.monotonic() instead of the deprecated asyncio.get_event_loop().time() pattern. AsyncMqttBridge: - Build ssl.SSLContext from fetched PEM via cadata and pass it with tls_set_context() instead of writing the CA to a temp file. Eliminates the file cleanup lifecycle and its crash-time leak. - _reconnect_loop catches all exceptions (not only OSError) so transport-specific failures like WebsocketConnectionError or ssl.SSLError no longer kill the background task silently. - Abnormal disconnects (reason_code.is_failure) log at WARNING. HomieDeviceConsumer: - _build_circuit: explicit -0.0 suppression replaces the cryptic `-raw or 0.0` idiom. - _derive_dsm_state: grid-exchanging check uses an epsilon (1.0 W) instead of != 0.0 float comparison, so DSM_OFF_GRID is reachable when lugs readings hover near zero.
- get_fqdn() returns str | None so callers can distinguish "no FQDN configured" (HTTP 404 or missing ebusTlsFqdn field) from an explicit empty string. Callers that treated "" as "not registered" must now check for None. - SpanPanelAPIError: remove custom __str__ override that silently truncated args[1:]; the default Exception.__str__ is more useful. - register_v2(): docstring warns that each call accumulates a new registered-client entry on the panel, so callers should persist and reuse the returned V2AuthResponse rather than re-registering on every restart.
NullLock monkey-patches a hardcoded list of paho-mqtt's internal *_mutex attributes. A paho minor-version refactor that renames or adds locks would silently break the override. Add a bidirectional check at module import: every attribute we list must exist on paho.Client, and no other *_mutex attribute may be present. Raise RuntimeError on drift (not assert, so python -O does not bypass it).
- Record 2.6.1 changes in CHANGELOG. - Remove "MQTT and simulation transports" / "REST polling or MQTT push" wording from protocol.py and models.py module docstrings. The simulation and REST transports were removed in 2.0.0.
There was a problem hiding this comment.
Pull request overview
Release v2.6.1 implementing follow-up fixes from the v2.6.0 review, focusing on MQTT robustness, TLS handling, and clearer semantics around configuration/registration.
Changes:
- Enforce
SpanMqttClient.snapshot_interval >= 1.0s(constructor + setter) and update debounce tests accordingly. - Refactor MQTT TLS setup to use an in-memory
SSLContext(no temp files), improve reconnect resilience/logging, and add a paho lock-layout drift check at import. - Change
get_fqdn()to returnstr | None(distinguish “not configured” vs explicit empty string) and update tests/docs/changelog.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates locked artifacts and bumps local package version to 2.6.1. |
| pyproject.toml | Bumps project version to 2.6.1. |
| CHANGELOG.md | Documents 2.6.1 behavior changes and fixes. |
| README.md | Documents new minimum snapshot interval behavior. |
| src/span_panel_api/mqtt/client.py | Enforces min snapshot interval, removes immediate-dispatch path, switches circuit-name wait to time.monotonic(), adjusts connection-callback logging. |
| src/span_panel_api/mqtt/connection.py | Builds TLS context in-memory, uses tls_set_context(), improves disconnect/reconnect logging and exception handling. |
| src/span_panel_api/mqtt/async_client.py | Adds import-time verification that paho lock attributes match the monkey-patch list. |
| src/span_panel_api/mqtt/homie.py | Adds epsilon-based grid-exchanging heuristic and explicit -0.0 suppression for circuit power. |
| src/span_panel_api/auth.py | Changes get_fqdn() return type/semantics; adds register_v2() docstring warning. |
| src/span_panel_api/exceptions.py | Removes SpanPanelAPIError.__str__ override. |
| src/span_panel_api/protocol.py | Updates module docstring to remove stale simulation transport references. |
| src/span_panel_api/models.py | Updates module docstring to remove stale REST/simulation wording. |
| tests/conftest.py | Updates MQTT bridge fixture to patch _build_ssl_context() instead of tempfiles. |
| tests/test_mqtt_debounce.py | Updates debounce and interval-validation tests for the >= 1.0s requirement. |
| tests/test_mqtt_connect_flow.py | Updates TLS expectations and avoids wall-clock waits by directly firing the snapshot timer. |
| tests/test_mqtt_client_connection.py | Updates expected log level for connection-callback exceptions. |
| tests/test_detection_auth.py | Updates/extends get_fqdn() tests for None vs missing field vs explicit empty string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rror connect() documents SpanPanelConnectionError / SpanPanelTimeoutError as its only raises. Two paths leaked unexpected exception types to callers: - _build_ssl_context(ca_pem) can raise ssl.SSLError or ValueError when the panel returns a malformed CA PEM. - The executor connect wrapper caught only OSError, but paho raises transport-specific failures that do not inherit from OSError — notably WebsocketConnectionError when transport='websockets'. Both are now wrapped in SpanPanelConnectionError with context.
The 1.0s floor was based on a misread of the integration constraint: the integration can be configured with scan interval 0 for real-time updates, so the library must not reject it. Restore the <=0 immediate-dispatch path and drop the ValueError validation from both __init__ and set_snapshot_interval(). Tests updated to cover real-time dispatch for interval=0 and interval<0. CHANGELOG and README reverted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Address findings from the v2.6.0 code review. Only one API-surface change (
get_fqdn()return type); no new runtime dependencies.Correctness & robustness
AsyncMqttBridgeTLS no longer touches disk — the panel's CA PEM is loaded viassl.SSLContext(cadata=…)and attached withtls_set_context(). The temp-file lifecycle (and its crash-time leak) is gone. Malformed PEM now wraps toSpanPanelConnectionError.OSError). Transport-specific failures —WebsocketConnectionError,ssl.SSLError,socket.gaierror— now surface asSpanPanelConnectionErroron the connect path and keep the reconnect loop alive.!= 0.0. Lugs readings rarely land exactly on zero, so theDSM_OFF_GRIDbranch is actually reachable now.get_fqdn()returnsstr | None—Nonemeans "no FQDN configured" (404 or missing field).""as "not registered" must switch tois None.WARNING(reason_code.is_failure); clean disconnects stay atDEBUG.WARNING(wasexception), matching_dispatch_snapshot.async_clientnow verifies both presence and absence of*_mutexattributes, raisingRuntimeErroron drift. Uses a real check, notassert(survivespython -O).Polish
_wait_for_circuit_namesusestime.monotonic()instead of the deprecatedasyncio.get_event_loop().time().-0.0inSpanCircuitSnapshot.instant_power_wsuppressed explicitly (replaces the cryptic-raw or 0.0).SpanPanelAPIError.__str__override removed — it truncatedargs[1:]with no benefit.register_v2()docstring warns about client-entry accumulation on the panel.protocol.pyandmodels.py(simulation and REST were cut in 2.0.0).Breaking changes
get_fqdn()signature is nowstr | None. Unconfigured state returnsNone(was"").Test plan
get_fqdn()contract)ruff check/ruff format --check/mypy src/clean